Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[theme] Built-in convertLength method #19720

Merged
merged 1 commit into from
Feb 18, 2020
Merged

[theme] Built-in convertLength method #19720

merged 1 commit into from
Feb 18, 2020

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Feb 15, 2020 •
edited
Loading

This is a continuation of #19638, one less dependency, among our long list: https://npm.anvaka.com/#/view/2d/%2540material-ui%252Fcore, with the CSS-in-JS module as a peer dependency in v5? It should be much better.

I think it's a good opportunity to continue the reflection on the best dependency policy Material-UI could follow. I think that we should be very strict in the way we accept dependencies. If we go back to the fundamentals, a dependency is about solving a problem we really need to and that we don't want to solve ourselves.

While Vuetify, ag-grid, Lodash, Vue, Bootstrap, or Angular follow a zero direct dependency policy (but accept peer dependencies), I wonder if it's not too strict.

Here are the dimension I think that we should consider in the equation that should increase the chance for adding a dependency:

  1. It's a hard problem to solve. For instance, CSS-in-JS.
  2. It doesn't align with the problems we try to solve and why users come to Material-UI. For instance, date-fns.
  3. It's a commonly used package in the ecosystem or has the potential for. For instance, hoist-non-react-statics.

If a dependency doesn't rank well within these dimensions, it's a good signal we should remove it, hence this pull request.

@mui-pr-bot
Copy link

Details of bundle changes.

Comparing: 0426817...96c9301

bundle Size Change Size Gzip Change Gzip
@material-ui/core[umd] ▼ -55 B (-0.02% ) 318 kB ▼ -27 B (-0.03% ) 92 kB
@material-ui/core ▼ -52 B (-0.01% ) 362 kB ▼ -49 B (-0.05% ) 98.9 kB
TablePagination ▼ -1 B (-0.00% ) 144 kB -- 42 kB
TableSortLabel -- 77.8 kB ▼ -44 B (-0.18% ) 24.4 kB
Avatar -- 65.6 kB ▲ +21 B (+0.10% ) 20.7 kB
@material-ui/lab -- 193 kB ▼ -8 B (-0.01% ) 57.2 kB
SwipeableDrawer -- 92.6 kB ▲ +2 B (+0.01% ) 28.9 kB
ExpansionPanel -- 72.7 kB ▲ +1 B (0.00% ) 22.7 kB
Rating -- 70.8 kB ▲ +1 B (0.00% ) 22.8 kB
SpeedDialIcon -- 64.9 kB ▼ -1 B (-0.00% ) 20.3 kB
@material-ui/styles -- 51.4 kB -- 15.4 kB
@material-ui/system -- 16.5 kB -- 4.32 kB
Alert -- 83.8 kB -- 26.3 kB
AlertTitle -- 64.5 kB -- 20.3 kB
AppBar -- 64.4 kB -- 20.2 kB
Autocomplete -- 132 kB -- 41.4 kB
AvatarGroup -- 62.6 kB -- 19.7 kB
Backdrop -- 68.2 kB -- 21.1 kB
Badge -- 65.7 kB -- 20.4 kB
BottomNavigation -- 62.7 kB -- 19.7 kB
BottomNavigationAction -- 75.9 kB -- 24 kB
Box -- 72.2 kB -- 21.9 kB
Breadcrumbs -- 68.1 kB -- 21.4 kB
Button -- 80.1 kB -- 24.5 kB
ButtonBase -- 74.4 kB -- 23.3 kB
ButtonGroup -- 83.6 kB -- 25.6 kB
Card -- 63.2 kB -- 19.8 kB
CardActionArea -- 75.5 kB -- 23.8 kB
CardActions -- 62.4 kB -- 19.6 kB
CardContent -- 62.3 kB -- 19.5 kB
CardHeader -- 65.4 kB -- 20.6 kB
CardMedia -- 62.7 kB -- 19.7 kB
Checkbox -- 83.3 kB -- 26.3 kB
Chip -- 83 kB -- 25.4 kB
CircularProgress -- 64.4 kB -- 20.3 kB
ClickAwayListener -- 3.91 kB -- 1.55 kB
Collapse -- 68.4 kB -- 21.2 kB
colorManipulator -- 3.88 kB -- 1.52 kB
Container -- 63.5 kB -- 19.9 kB
CssBaseline -- 62.3 kB -- 19.6 kB
Dialog -- 83.4 kB -- 26 kB
DialogActions -- 62.4 kB -- 19.6 kB
DialogContent -- 62.6 kB -- 19.6 kB
DialogContentText -- 64.4 kB -- 20.2 kB
DialogTitle -- 64.6 kB -- 20.3 kB
Divider -- 63 kB -- 19.8 kB
docs.landing -- 56.4 kB -- 14.5 kB
docs.main -- 604 kB -- 196 kB
Drawer -- 85.2 kB -- 25.9 kB
ExpansionPanelActions -- 62.4 kB -- 19.6 kB
ExpansionPanelDetails -- 62.3 kB -- 19.5 kB
ExpansionPanelSummary -- 78.5 kB -- 24.8 kB
Fab -- 77.2 kB -- 24.1 kB
Fade -- 23.6 kB -- 8.01 kB
FilledInput -- 73.9 kB -- 23 kB
FormControl -- 64.8 kB -- 20.2 kB
FormControlLabel -- 65.9 kB -- 20.6 kB
FormGroup -- 62.3 kB -- 19.6 kB
FormHelperText -- 63.7 kB -- 20 kB
FormLabel -- 63.8 kB -- 19.8 kB
Grid -- 65.4 kB -- 20.5 kB
GridList -- 62.8 kB -- 19.7 kB
GridListTile -- 64.1 kB -- 20.1 kB
GridListTileBar -- 63.6 kB -- 19.9 kB
Grow -- 24.2 kB -- 8.22 kB
Hidden -- 66.3 kB -- 20.8 kB
Icon -- 63.1 kB -- 19.8 kB
IconButton -- 76.5 kB -- 23.9 kB
Input -- 72.8 kB -- 22.8 kB
InputAdornment -- 65.4 kB -- 20.6 kB
InputBase -- 70.9 kB -- 22.3 kB
InputLabel -- 65.7 kB -- 20.5 kB
LinearProgress -- 65.7 kB -- 20.5 kB
Link -- 67 kB -- 21.1 kB
List -- 62.7 kB -- 19.6 kB
ListItem -- 77.5 kB -- 24.2 kB
ListItemAvatar -- 62.4 kB -- 19.5 kB
ListItemIcon -- 62.5 kB -- 19.6 kB
ListItemSecondaryAction -- 62.3 kB -- 19.5 kB
ListItemText -- 65.3 kB -- 20.5 kB
ListSubheader -- 63.1 kB -- 19.8 kB
Menu -- 89.1 kB -- 27.4 kB
MenuItem -- 78.6 kB -- 24.5 kB
MenuList -- 66.4 kB -- 20.8 kB
MobileStepper -- 68.2 kB -- 21.4 kB
Modal -- 14.5 kB -- 5.04 kB
NativeSelect -- 77.2 kB -- 24.3 kB
NoSsr -- 2.19 kB -- 1.04 kB
OutlinedInput -- 74.9 kB -- 23.3 kB
Pagination -- 85.5 kB -- 26.3 kB
PaginationItem -- 81.2 kB -- 25 kB
Paper -- 62.7 kB -- 19.6 kB
Popover -- 83.5 kB -- 25.8 kB
Popper -- 28.8 kB -- 10.3 kB
Portal -- 2.92 kB -- 1.3 kB
Radio -- 84.4 kB -- 26.6 kB
RadioGroup -- 64.8 kB -- 20.1 kB
RootRef -- 4.24 kB -- 1.64 kB
ScopedCssBaseline -- 63.2 kB -- 19.9 kB
Select -- 117 kB -- 34.6 kB
Skeleton -- 63.3 kB -- 20 kB
Slide -- 25.7 kB -- 8.74 kB
Slider -- 77 kB -- 24.2 kB
Snackbar -- 75.7 kB -- 23.7 kB
SnackbarContent -- 63.9 kB -- 20.1 kB
SpeedDial -- 86.6 kB -- 27.3 kB
SpeedDialAction -- 119 kB -- 37.6 kB
Step -- 63 kB -- 19.8 kB
StepButton -- 82.7 kB -- 26.2 kB
StepConnector -- 63.1 kB -- 19.9 kB
StepContent -- 69.5 kB -- 21.8 kB
StepIcon -- 65 kB -- 20.3 kB
StepLabel -- 69 kB -- 21.7 kB
Stepper -- 65.2 kB -- 20.6 kB
styles/createMuiTheme -- 16.6 kB -- 5.85 kB
SvgIcon -- 63.4 kB -- 19.8 kB
Switch -- 82.5 kB -- 26 kB
Tab -- 76.7 kB -- 24.3 kB
Table -- 62.9 kB -- 19.7 kB
TableBody -- 62.4 kB -- 19.5 kB
TableCell -- 64.4 kB -- 20.3 kB
TableContainer -- 62.3 kB -- 19.5 kB
TableFooter -- 62.4 kB -- 19.5 kB
TableHead -- 62.4 kB -- 19.5 kB
TableRow -- 62.8 kB -- 19.7 kB
Tabs -- 85.8 kB -- 27.2 kB
TextareaAutosize -- 5.12 kB -- 2.14 kB
TextField -- 126 kB -- 36.6 kB
ToggleButton -- 76.5 kB -- 24.2 kB
ToggleButtonGroup -- 63.5 kB -- 20 kB
Toolbar -- 62.7 kB -- 19.7 kB
Tooltip -- 103 kB -- 32.4 kB
TreeItem -- 74.3 kB -- 23.5 kB
TreeView -- 67 kB -- 21.1 kB
Typography -- 64 kB -- 20 kB
useAutocomplete -- 14.7 kB -- 5.31 kB
useMediaQuery -- 2.58 kB -- 1.06 kB
Zoom -- 23.6 kB -- 8.12 kB

Generated by 🚫 dangerJS against 96c9301

@oliviertassinari oliviertassinari added the core Infrastructure work going on behind the scenes label Feb 16, 2020
@eps1lon
Copy link
Member

eps1lon commented Feb 16, 2020

It all comes down to: What problem are you trying to solve. You haven't answered that.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Feb 16, 2020 •
edited
Loading

In the case of this dependency:

  1. goes against having it, the diff illustrates it, the logic is simple
  2. it's unsure, the closest discussion we have on this topic in Include styledBy in csskit package #14812 with a @material-ui/csskit package
  3. goes against having it, they are few dependents and downloads outside MUI.

I think that 1. and 3. outweigh 2. It seems to be a win.

@oliviertassinari
Copy link
Member Author

It all comes down to: What problem are you trying to solve. You haven't answered that.

@eps1lon When a dependency solves a simple problem, is not frequently used in the ecosystem, and embodies a problem developers come to Material-UI for, what would be the advantage to keep it?

I doubt we should consider the "reduce bundle size" argument per the "not frequently used in the ecosystem" (non-signficant impact, on average).

Giving more nuances to the previous arguments around the why:

  1. It gives us more flexibility (we can refactor, change the bundling strategy, change the browser support, drop features, etc.). I have a lot of gratitude for the work @TrySound did with adding esm support to Material-UI's dependencies. If it wasn't necessary in the first place, it would have been even better.
  2. It enables us to internalize core problems we can try to solve really well. For instance, it's why I wanted to bring a headless autocomplete component over react-select.
  3. It resonates with the fear developers have when introducing new dependencies. it feels safer, even if in practice, I think that it's only a perception. However, most "buy" decisions are irrational, as much as I wish we could ignore feelings, I really doubt we can.

@oliviertassinari oliviertassinari merged commit f2d74e9 into mui:master Feb 18, 2020
@oliviertassinari oliviertassinari deleted the host-convert-css-length branch February 18, 2020 10:12
This was referenced Mar 10, 2020
Merged
Closed
Open
This was referenced Mar 21, 2020
Merged
Open
Closed
This was referenced Apr 16, 2020
Merged
Closed
@snyk-bot snyk-bot mentioned this pull request May 4, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@oliviertassinari @mui-pr-bot @eps1lon